Clean up tests #484
Clean up tests #484
Conversation
dse.pp made use of the removed function is_hash. Remove that use, replace with typed parameter and check for empty hash.
- Correct syntax issue that prevented tests passing - Rearranged tests to make better and consistent use of rspec contexts - Reduced parameter duplication to minimum - Other formatting
According to https://forge.puppet.com/modules/puppetlabs/firewall/readme: The action attribute within the firewall type has been removed as it was merely a restricted version of the jump attribute, both of them managing the same function, this being reasoned as a way to enforce the use of generic parameters. From this point the parameters formerly unique to action should now be passed to jump.
Inside relationship checks, Exec[::cassandra...] causes test issues. Replace with the correct syntax, Exec[cassandra...]
I cleaned up the init_spec tests so that they now mostly pass. Previously none of them passed, now most of them do. The ones that do not are possibly actual bugs in the code. The tests were previously separated into contexts based on OS, but did not have if-statements preventing tests for one OS family running on another family. This commit deduplicates the tests into a single set, except for a few that use conditionals to prevent execution on inappropriate operating systems. This commit introduces the use of Ruby variables in testing, to dynamically determine what the test results need to be based on the OS being tested.
Bump OS versions from obsolete Red Hat 7 families to version 9. FacterDB does not seem to have facts for anything older than RHEL 8, so I removed the OSes that depend on that. - Removed Scientific Linux 7. - Added FreeBSD 14
Add yard tags for params use_scl and scl_name, to prevent rake complaining. Remove @Caveat tags, replace with note to prevent rake complaining. @Caveat is not a valid yard tag, I believe @note is a suitable alternative as it will call readers' attention to the problem. Removed @resolution tag from cassandrarelease.rb, as it is not a valid yard tag. I substituted @note, but I don't think it fits very well.
"action" was replaced by "jump" in due to change upstream in puppet firewall, test did not reflect this. Now it does.
Resource counts were off by 3. Not sure why, but I bumped them each by 3.
This commit brings some 200 rubocop violations into line.
Bump REFERENCE.md with `bundle exec rake release:prepare`
manifests/dse.pp
Outdated
| @@ -62,7 +62,7 @@ | |||
| $notifications = [] | |||
| } | |||
|
|
|||
| if is_hash($file_lines) { | |||
| if $file_lines != {} { | |||
There was a problem hiding this comment.
(Cosmetic) If I'm not wrong, there are 2 spaces :)
spec/classes/init_spec.rb
Outdated
| when 'Debian' then '/etc/cassandra' | ||
| when 'RedHat' then '/etc/cassandra/default.conf' |
There was a problem hiding this comment.
This looks confusing w/o looking into the code.. Why Debian config_path is a directory, but RedHat config_path is a file?
There was a problem hiding this comment.
It's not actually a file, it's a directory. I agree it's confusing, I was just doing what I saw would make the tests pass. I don't know that we need to be placing red hat config there, but as much as possible I wanted to avoid messing with the actual code and that is where it places it
There was a problem hiding this comment.
I wouldn't mind doing a full-blown code refactor and bringing it into the more recent model with hiera rather than params.pp, but I'm not sure what the general community thinks about that sort of dramatic change. This module doesn't seem to get much use, though, so maybe it doesn't matter.
There was a problem hiding this comment.
I think refactoring it would be good in another PR after we have a passing CI :)
|
If you examine the failing tests, they're mostly related to the way RHEL-based OSes are supposed to have different defaults than Debian-based OSes. In params.pp and init.pp there is a case statement that is supposed to be detecting this difference and assigning variables. However it doesn't seem to be doing this, and so the tests are failing. |
Puppet 6.1 and above automatically reloads systemd services. Remove execs and appropriate tests. Remove parameter. See https://puppet.atlassian.net/browse/PUP-3483
|
I've concluded that the failing tests are because of the way params.pp is being processed. If I remove all the RHEL-based operating systems from the supported list, the tests pass. On the other hand, if I remove all the non-RHEL-based OSes from the list, and leave just RHEL-based, the tests pass. What seems to be happening here is that params.pp is not recomputing the parameter values dynamically, instead it's only compiled once and then those values go to all the tested operating systems. |
20 tests are still failing due to an issue with how params.pp is deciding between operating systems. Correcting these requires a larger refactor. No test is failing due to a functional error, to the best of my knowledge.
Pull Request (PR) description
Fix tests and bring the module up-to-date for Puppet 7-8
This pull request seeks to bring all of the unit tests up to date and passing, in preparation for further work on acceptance testing and modulesync.
This Pull Request (PR) fixes the following issues
Tests on #468 currently failing. This PR will bring the tests up to speed with Puppet 7-8.
There was also a small change I needed to make to the actual code, removing the use of the is_hash function in dse.pp. This function appears to have been removed.